Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPC-9975 Remove use-query-params #481

Merged
merged 11 commits into from
Jan 21, 2025
Merged

HPC-9975 Remove use-query-params #481

merged 11 commits into from
Jan 21, 2025

Conversation

Onitoxan
Copy link
Contributor

@Onitoxan Onitoxan commented Dec 16, 2024

Nature of PR

  • Bug-fix
  • Hot-fix
  • 🟢 Feature
  • Testing
  • 🟢 Package update
  • CI

Description

This PR is related to HPC-9965, read for more context.

In this PR we focus on removing use-query-params package. It became obsolete since with the use of react-router data API hooks (precisely, useSearchParams()) and using io-ts to write runtime validation, we can achieve the same objective.

We do this in order to reduce dependency of external libraries.

How to test changes

To test changes I would try to manipulate the URL to see how it reacts even to unexpected params and with wrong data on it.

Beside this, I would focus mainly on reading logic for useQueryParams() to verify it really covers corner cases

@Onitoxan Onitoxan added the ready for review All comments have been addressed, and the Pull Request is ready for review label Dec 16, 2024
@Onitoxan Onitoxan requested a review from a team as a code owner December 16, 2024 09:12
@Onitoxan Onitoxan changed the title HPC-9975 Remove use-query-params HPC-9975 Remove use-query-params Dec 16, 2024
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

Base automatically changed from HPC-9965 to develop January 9, 2025 11:54
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

@Pl217 Pl217 assigned Onitoxan and unassigned Pl217 Jan 10, 2025
@Pl217 Pl217 added needs minor changes There are review or issue comments to address and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Jan 10, 2025
The proposed solution is to use `setTimeout`, reseting the form
and changing the URL are two actions that trigger a re-render.
In order to avoid missbehaviours on how they get added to React's
event queue, we add a `setTimeout` so the execution is done in the
next tick.
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

@Onitoxan Onitoxan assigned Pl217 and unassigned Onitoxan Jan 14, 2025
@Onitoxan Onitoxan added ready for review All comments have been addressed, and the Pull Request is ready for review blocked This PR is blocked on other work and removed needs minor changes There are review or issue comments to address labels Jan 14, 2025
@Onitoxan
Copy link
Contributor Author

This PR is blocked by ticket HPC-10019, once the work in here is merged we can proceed on merging this PR. It can be reviewed in the meantime.

Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

@Onitoxan Onitoxan added the pending prior merge Another Pull Request needs to be merged before this one label Jan 14, 2025
Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

@Onitoxan Onitoxan removed pending prior merge Another Pull Request needs to be merged before this one blocked This PR is blocked on other work labels Jan 20, 2025
Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed you said "try manipulating the URL" in the initial comment and when I did, you said in the inline comments that no one will change the URL manually :P

apps/hpc-ftsadmin/src/app/utils/codecs.ts Outdated Show resolved Hide resolved
apps/hpc-ftsadmin/src/app/utils/codecs.ts Outdated Show resolved Hide resolved
@Onitoxan
Copy link
Contributor Author

I noticed you said "try manipulating the URL" in the initial comment and when I did, you said in the inline comments that no one will change the URL manually :P

I should have been more specific, no user will manipulate the URL, but nonetheless, us developers we should make sure that nothing too "broken" happens in any case :)

Copy link

@unocha-hpc unocha-hpc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks have passed and this pull request is ready for manual review

@Onitoxan Onitoxan assigned Onitoxan and unassigned Pl217 Jan 21, 2025
@Onitoxan Onitoxan added ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished. and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Jan 21, 2025
@Onitoxan Onitoxan merged commit 3f2ba3c into develop Jan 21, 2025
3 checks passed
@Onitoxan Onitoxan deleted the HPC-9975 branch January 21, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for merge Review and testing is complete. It is ready for merging as soon as CI has finished.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants